Skip to content

Conversation

archdragon
Copy link
Contributor

@archdragon archdragon commented Jun 18, 2019

Add reference popovers

image

Summary

Adds popovers discussed in: #971 - tooltips with extra info that appear after hovering over a link, Wikipedia style. :)

Demo

Demo Link (After fixes)

[ Old Demo Link ]

1. What triggers a reference popover

Reference popover will appear after you hover over a:

  • ➡️ Link to a function/callback - function signature and summary will be displayed
    image

  • ➡️Module - module name and summary will be displayed
    image

  • ➡️Types

    • ➡️If defined in the module - name and signature will be displayed
      image

    • ➡️If defined on the typespecs page

      • ➡️For built-in types - popover will extract detailed info
        image

      • ➡️For basic types and literals - basic hint will be displayed
        image

2. Handling types

I'm able to show info about the built-in types, because they are displayed as a neat table - so it's easy to find the requested type and the corresponding definition.

For basic types and literals this is not really possible - their definitions are unstructured and displayed as one big <code> block.

image

Maybe we could display them in a table instead? Then I'll be able to upgrade popovers descriptions. :)

3. Handling external links

External links - communication between different domains/projects - is fully supported.

For example if https://hexdocs.pm/elixir contains the popover code, links to https://hexdocs.pm/elixir(...) will trigger appropriate popovers.

4. Security

Incoming data is always displayed as plain text to guard against potential malicious payload.

5. Switching on and off

Tooltips can be disabled with the link at the bottom of the page. Same as with Night Mode, user's choice is stored in localStorage.

image

6. Delay

Popovers will appear after a short delay. This way they won't get trigger on accident (ie. while scrilling the page or just moving cursor around).

7. How it's implemented

Script is based on José's suggestion to use an iframe to display the popover content.

I've started with a basic iframe displaying a simplified version of the requested page. That worked great for smaller, simpler functions. Longer function names/signatures were overflowing and iframe resizing was needed. In addition iframe content was sometimes "jumping" even after the page was fully loaded (I suspected some other JS scripts adding styles and causing content to shift).

At this point I was already sending messages (containing size measurements and load progress info) from the iframe to the parent page - I've decided to try extracting tooltip title and description using JS and sending them to the parent via postMessage.

This is how it works right now:

  1. User hovers over a link
  2. A param '?hint=true' is added to the url.
  3. Url is loaded inside the iframe
  4. When '?hint=true' is detected Javascript tries to find name and description of the module/function/type requested in the url.
  5. If it's successful it send them to the parent via window.postMessage
  6. Parent receives the postMessage and displays the popover

@archdragon archdragon mentioned this pull request Jun 18, 2019
@wojtekmach
Copy link
Member

Just wanted to say this is amazing.

Couple minor points:

image

(http://krowinski.com/ex_doc/popovers_done/elixir/GenServer.html#module-example)

This is unfortunate result of security considerations above, we rightfully so don't interpret any markup and so we can't do indentation, new lines etc. What if for each spec DOM element we store a data attribute with spec in plaintext (with spaces and newlines) and use that to render in popover? This would obviously increase the size of the docs unfortunately.

What do you think about making it possible to hover over content of the popover? (Currently it's not possible.) This way we'd be able to copy-paste stuff, e.g. complicated specs. Right now what we can do is click on the link and copy-paste the contents from the page we land on.

Both issues are minor and if anything we should work on them in future PRs.

@josevalim
Copy link
Member

This is awesome! ❤️

Here are some general comments:

  1. [discussion] Should we show popovers for the current page itself? For example, Kernel shows popovers for Kernel.

  2. [bug] Currently, every time a popover is shown, we add a new entry to the browser's history (or at least we do so in Firefox)

  3. [discussion] the type stuff is really nice but part of me says we should keep it simple and simply show "Basic / built-in type" with no description of it. I.e. we just look if the link is to "typespecs.html" and if so, we show a short description.

  4. [discussion] One of the features we want to have from popovers is to show popovers across projects. Therefore I wonder if the popover itself should include some information about the project the popover is from, possibly in small font, either in the header or in the footer. Such as "Elixir v1.9.2". I believe this information can be found in the meta tag.

expect(extractModuleHint(modulePageObject).kind).to.eql('module')
})

it('extracts plain text, without html tags', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed? :D

}
// Tooltip will appear only if the mouse cursor stays on the link for at least 150ms.
// This way tooltips will not appear if we are scrooling the page or just moving the cursor around.
const hoverDelayTime = 150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is a loading time, I think we should be fine with 100 here. :D (bike shedding)

@archdragon
Copy link
Contributor Author

Thanks @wojtekmach , @josevalim !

This is unfortunate result of security considerations above, we rightfully so don't interpret any markup and so we can't do indentation, new lines etc. What if for each spec DOM element we store a data attribute with spec in plaintext (with spaces and newlines) and use that to render in popover? This would obviously increase the size of the docs unfortunately.

Actually adding newline support should be super easy - I'm not removing inner whitespaces, just html tags. A white-space: pre CSS should fix this!

What do you think about making it possible to hover over content of the popover? (Currently it's not possible.) This way we'd be able to copy-paste stuff, e.g. complicated specs. Right now what we can do is click on the link and copy-paste the contents from the page we land on.

Both issues are minor and if anything we should work on them in future PRs.

👍 I'll need to experiment with it a little bit. This I would definitely move to another PR, since it requires a bit of tinkering to get right :)

  1. [discussion] Should we show popovers for the current page itself? For example, Kernel shows popovers for Kernel.

Personally I would vote for disabling them.

At first I thought that, if I'm a newbie, a quick summary of the current module can still be useful. But after checking again I can see that current module is referenced mostly at the very top of the page, in the first description block - and popover does not make much sense there.

  1. [bug] Currently, every time a popover is shown, we add a new entry to the browser's history (or at least we do so in Firefox)

On it!

  1. [discussion] the type stuff is really nice but part of me says we should keep it simple and simply show "Basic / built-in type" with no description of it. I.e. we just look if the link is to "typespecs.html" and if so, we show a short description.

I wanted to see what I can do with types, but I feel that simpler tip might indeed be better.
a) Removing built-in types extraction will greatly simplify the code.
b) Typespecs tips will look a bit more consistent

  1. [discussion] One of the features we want to have from popovers is to show popovers across projects. Therefore I wonder if the popover itself should include some information about the project the popover is from, possibly in small font, either in the header or in the footer. Such as "Elixir v1.9.2". I believe this information can be found in the meta tag.

👍I'll experiment with the styles/positioning and prepare a demo.

@cybrox
Copy link
Contributor

cybrox commented Jun 19, 2019

  1. [discussion] Should we show popovers for the current page itself? For example, Kernel shows popovers for Kernel.

I would also vote for disabling them, they get excessive quite fast in modules that reference lots of their own types/functions.

  1. [discussion] One of the features we want to have from popovers is to show popovers across projects. Therefore I wonder if the popover itself should include some information about the project the popover is from, possibly in small font, either in the header or in the footer. Such as "Elixir v1.9.2". I believe this information can be found in the meta tag.

I like this.

pathnameEnd: '/typespecs.html',
categories: [
{ name: 'basicType', description: 'Basic type', hash: '#basic-types', detailsAvailable: false },
{ name: 'literal', description: 'Literal', hash: '#literals', detailsAvailable: false },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ExDoc doesn't link typespec literals, do you have any examples of this tooltip showing? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ExDoc doesn't link typespec literals, do you have any examples of this tooltip showing? 🙂

Ah, that makes sense! I've not seen them anywhere, but added this category just in case, but I guess I can safely remove it. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the plan is to remove the typespec handling anyway, we don't have to worry about this particular bit either. :D

@archdragon
Copy link
Contributor Author

archdragon commented Jul 4, 2019

I've just uploaded the updates and fixes :)

➡️ New demo: Demo Link (new one in next post)

Changelog:

1. Removed type extraction for built-in and basic types.

Now only a simple hint is being shown.
image

2. Links to the current module page will not trigger the popover.

image
^ No popover when hovering over Kernel

3. Added version info

Displayed in the top right corner.

image

4. Fixed multiple history entries

Tooltips no longer add new history entries.

5. Adjusted delay

Basic delay between hover and tooltip showing up changed from 150 do 100ms.

6. Fixed white space handling in long signatures

Formatting is now preserved:

image

7. Minor style adjustments and fixes :)

@josevalim
Copy link
Member

Absolutely beautiful! ❤️ 💚 💙 💛 💜

@josevalim
Copy link
Member

josevalim commented Jul 5, 2019

I think I have found two, hopefully tiny, bugs:

  • Pop-over does not work when we are linking to a lesser arity. To reproduce it:

    1. Go to this page: http://krowinski.com/ex_doc/popovers_with_fixes/elixir/Access.html
    2. Search for "See the functions key/1, key!/1, elem/1, and all/0 for some of the available accessors."
    3. Mouse over "key/1", it will show the module documentation and not the documentation for key/1. Note that key/1 exists but it is simply a reference to key/2. Perhaps the current code does not work with references like that?
  • References to URLs with weird characters do not work. To reproduce:

    1. Go here: http://krowinski.com/ex_doc/popovers_with_fixes/elixir/Enum.html#sort_by/3
    2. Mouse over Kernel.<=/2 and it will show the docs for Kernel. I believe this is caused by the escaping in the URL :)

@cybrox
Copy link
Contributor

cybrox commented Jul 5, 2019

For 2., I suspect this is because of this bit. Back when I solved an issue with encoded/unencoded anchors, we decided to support both, however, the unencoded version is stored in a span element inside .detail so $hash.detail will not match if the hash is not encoded. The hash would need to be urlencoded to always work with the section id.

<section class="detail" id="%3C=/2">
    <span id="<=/2"></span>

Edit: The same also applies to 1.. The section gets the anchor with it's actual arity (/2) and the other arities are added as span's with ID's. Maybe we could adjust these generated span's with a alias class, so that they are identifiable and @archdragon can use them for selecting their parent .description

@archdragon
Copy link
Contributor Author

I think I have found two, hopefully tiny, bugs

For 2., I suspect this is because of this bit. Back when I solved an issue with encoded/unencoded anchors, we decided to support both, however, the unencoded version is stored in a span element inside .detail

Thanks for the info - should be a quick fix! :)

@archdragon
Copy link
Contributor Author

I've fixed both reported issues :)

➡️ New demo: Demo Link (new one in next post)

  1. Fixes the arity links

Note that key/1 exists but it is simply a reference to key/2. Perhaps the current code does not work with references like that?

Exactly - I missed the fact that we create separate anchors for that.

Now it's fixed. :)

Maybe we could adjust these generated span's with a alias class, so that they are identifiable

Sounds good, but that was not necessary - they are always direct descendants of the .description and that makes them easy to find. :)

image
image

  1. Fixes encoding/links to weird characters

image

@josevalim josevalim merged commit 8176680 into elixir-lang:master Jul 11, 2019
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@michallepicki
Copy link
Contributor

This looks cool! I have one idea though: wouldn't it be more useful if the displayed version was the since metadata instead of the current version? The current version is always displayed on the page in the same spot. If since is not present, we could leave this space empty

@archdragon
Copy link
Contributor Author

This looks cool! I have one idea though: wouldn't it be more useful if the displayed version was the since metadata instead of the current version? The current version is always displayed on the page in the same spot. If since is not present, we could leave this space empty

Thanks @michallepicki

Version info was added because tooltips work across projects (provided that both projects use the latest ex_doc version).

I think since would work great when the project references itself, but I'm not sure how to handle that cross-project without making the info confusing.

@josevalim
Copy link
Member

@archdragon one option is: if since is available, we show "since Elixir v1.8.0", otherwise we show "seen on Elixir v1.8.0" or similar. WDYT?

@michallepicki
Copy link
Contributor

@archdragon cool, where can I see it in action? Here hovering over ExUnit.DocTest doesn't do anything for me: https://hexdocs.pm/elixir/master/writing-documentation.html#doctests , similarily here Logger stuff: https://hexdocs.pm/elixir/master/library-guidelines.html#avoid-macros

@michallepicki
Copy link
Contributor

@archdragon I just re-read your response and realized - both need to use latest ex_doc, understood

@wojtekmach
Copy link
Member

wojtekmach commented Jul 11, 2019

ExUnit.DocTest doesn't do anything for me: https://hexdocs.pm/elixir/master/writing-documentation.html#doctests

the reason is ExUnit.DocTest link goes to https://hexdocs.pm/ex_unit/ExUnit.DocTest.html (which was generated with older ex_doc version). You can see it in action here: https://hexdocs.pm/elixir/master/Kernel.html as long as you hover over something that stays within Elixir docs (and not e.g. Mix)

@archdragon
Copy link
Contributor Author

@archdragon one option is: if since is available, we show "since Elixir v1.8.0", otherwise we show "seen on Elixir v1.8.0" or similar. WDYT?

Makes sense!

I'll need to spend just a bit more time on it to make sure it looks good. With since/seen on version name will occupy more space and I might need to adjust CSS rules a bit to account for that.

@josevalim
Copy link
Member

@archdragon one option is to do this: "Elixir v1.8.0" (without since) and "Elixir v1.3.0+" (with since).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants